-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix oppushdata overflow bug #22
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
- Coverage 78.28% 75.88% -2.40%
==========================================
Files 16 16
Lines 1165 846 -319
==========================================
- Hits 912 642 -270
+ Misses 166 140 -26
+ Partials 87 64 -23
Continue to review full report at Codecov.
|
Thanks for raising this @caevv. From an initial glance, I don't see how your PR changes fix the issue raised. Can you give us some more information regarding the panic situation? Input values so that we can reproduce the panic error you mention above would be great. |
You are right, there is more to it than that. The size of that I'm using this with tx id:
panics on
|
Regarding this last comment about using binary.LittleEndian.Uint64(), this is incorrect. If you did make this change, we would read 8 bytes from the script rather than the 2 that we need for a uint16.
Happy to explain further, if required.
Regards
Simon
… On 6 May 2021, at 10:19, Mark Smith ***@***.***> wrote:
@theflyingcodr commented on this pull request.
In bscript/oppushdata.go <#22 (comment)>:
>
case OpPUSHDATA2:
if len(b) < 3 {
return r, errors.New("not enough data")
}
- l := binary.LittleEndian.Uint16(b[1:])
- if len(b) < int(3+l) {
+
+ l := int(binary.LittleEndian.Uint16(b[1:]))
Would probably be better to use binary.LittleEndian.Uint64(b[1:]) here and it should resolve this issue without needing to convert to int etc
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#22 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAROAU2XC6GAMNH4F6YO3LTMJGJ5ANCNFSM44FKVGMQ>.
|
Good point @ordishs; I think the other approach is probably best then. Would you be able to add a unit test in with the failing script to show that the fix works and to ensure if this code is changed in future that the test catches it. |
fix panic:
panic: runtime error: slice bounds out of range [3:2]